refactor: move initial-state getters to native_account#3218
Conversation
Move get_initial_commitment, get_initial_storage_commitment, get_initial_vault_root, get_initial_item, get_initial_map_item, get_initial_asset and get_initial_balance from the active_account module to native_account. These procedures operate on the transaction's native account, so they now assert the active account is native (via memory::assert_native_account) and panic with ERR_ACCOUNT_IS_NOT_NATIVE when invoked from a foreign procedure invocation (FPI) context. Removes the is_native_account conditional logic from the kernel get_initial_* procedures and the now-unused branching pointer helpers get_account_initial_storage_slots_ptr and get_account_initial_vault_root_ptr. Updates all standards/auth callers, the mock account code, kernel and FPI tests, protocol library docs and the changelog.
- Re-export get_initial_commitment/storage_commitment/vault_root as pub use aliases of the underlying memory procedures instead of one-line wrappers. - Remove the redundant 'assumed to be invoked after asserting native' doc comments in account.masm. - Restore the foreign-account read tests to verify correct values via the active_account current getters (get_item/get_map_item/get_balance), since foreign accounts are immutable. - Note in protocol_library.md that foreign accounts are immutable and their initial values should be read via active_account procedures. - Shorten the changelog entry to reference get_initial_*.
…gorize-initial-getters
Fumuran
changed the title
native_account
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
Did not re-review the procedures in crates/miden-protocol/asm/protocol/src/native_account.masm in detail, assuming they are copies.
mmagician
left a comment
mmagician
left a comment
There was a problem hiding this comment.
LGTM modulo the now-incorrect docs
One more general comment, unrelated to this PR: I wonder if executing account would be a better name than native account. I remember the naming had confused me quite a bit in the past: native to what?
The idea under the "native" name was that the account, against which the transaction is being executed, is native to this transaction, we always begin to execute against it — opposite to the foreign accounts which are just loaded into the memory. I don't have a strong opinion here, but "executing account" could be a bit misleading: during the FPI we are executing against the foreign account, so in some way it in turn becomes an executing account (or at least somebody can think of it like that). I recall that we have this discussion, so the native account turned out to be maybe not great, but the compromise name. But anyway, it will be good if we will be able to come up with something more self-explanatory. |
| # check that this procedure was executed against the native account | ||
| exec.memory::assert_native_account | ||
| # => [slot_id_suffix, slot_id_prefix, pad(14)] | ||
|
|
||
| # authenticate that the procedure invocation originates from the account context | ||
| exec.authenticate_account_origin |
There was a problem hiding this comment.
Question: here we call authenticate_account_origin after we've verified that the current account is the native account, but we don't do this in other procedures. For example, account_get_initial_storage_commitment doesn't run authenticate_account_origin. I'm not 100% sure if there is some motivation behind this (cc @PhilippGackstatter - maybe you remember?).
Could we create a table listing account procedures together with the context from which they can be executed?
There was a problem hiding this comment.
Probably it will make sense to create such table in #3204, because this PR adds this account context requirement to almost every account procedure.
6 participants
Summary
Closes #2034.
Recategorizes the seven initial-state account getters from
miden::protocol::active_accounttomiden::protocol::native_account. Because the concept of an "initial" (beginning-of-transaction) state is only meaningful for the transaction's native account, these procedures now always operate on the native account and panic withERR_ACCOUNT_IS_NOT_NATIVEwhen invoked from a foreign procedure invocation (FPI) context, rather than silently branching on native vs. foreign.Moved procedures:
get_initial_commitmentget_initial_storage_commitmentget_initial_vault_rootget_initial_itemget_initial_map_itemget_initial_assetget_initial_balanceChanges
active_account.masmintonative_account.masm(with the required offset/asset imports).api.masm): addedexec.memory::assert_native_accountto the six initial-getter procs, matching the existing native-only guard pattern (e.g.account_incr_nonce,account_set_item).account.masm): removed theis_native_accountbranches from the commitment / storage-commitment / vault-root getters, and switched the item / map-item / asset getters (andget_item_patch) to native-only pointers.memory.masm: removed the now-unused branching helpersget_account_initial_storage_slots_ptrandget_account_initial_vault_root_ptr.singlesig,singlesig_acl,multisig,multisig_smart,signature,note_/tx_script_allowlist,no_auth,network_account) and the mock account code tonative_account::, fixinguseimports per file.test_account.rs; rewrote the two FPI tests so foreign-context calls now assertERR_ACCOUNT_IS_NOT_NATIVE.protocol_library.mdand added a[BREAKING]changelog entry.Design note
Per the discussion, FPI invocation of these getters is now a hard error (via
memory::assert_native_account). This technically adds an assert rather than purely removing conditional logic, but it is consistent with how other native-only procedures (set_item,incr_nonce,add_asset) are already guarded, and it removes the dual-behavior branches the issue called out.Testing
cargo build -p miden-protocol -p miden-standards(compiles the MASM kernel and libraries).cargo test -p miden-testing: 30test_accounttests, 12test_fpitests (including two new error tests), and 81 auth integration tests — all passing.🤖 Generated with Claude Code